Add instrumentation and expose measurement in prometheus format #210

StevenYCChou · 2018-11-15T19:40:00Z

Add interface inside api_server, so we expose instrumentation metrics later on (currently working on it).

Some thoughts:

I added a private function MetadataApiServer::SerializeMetricsToPrometheusTextFormat() so we can test the functionality without considering http_connection, which isn't tested in current test suites.
I only built prometheus-cpp core library with gzip compression disabled for 2 reason: 1) gzip is not used in our http server, 2) gzip compression requires system library unless we statically compile and link with gzip library.

davidbtucker

LGTM

src/Makefile

src/api_server.cc

test/Makefile

test/api_server_unittest.cc

StevenYCChou · 2018-11-16T17:01:39Z

I noticed we use a lot of space indentation in Makefile, though I think only tab indentation should be used in Makefile. Will use separate PR to change that indentation.

igorpeshansky

Can you please add at least one actual metric to this PR? I understand that it'll make the PR larger, but it would also make it easier to see what the abstractions are and how we plan to track metrics in the client code. Also note that it's not easy to understand what "exposition" means in this context.

src/Makefile

src/api_server.cc

igorpeshansky · 2018-11-15T21:19:00Z

src/api_server.cc

+  conn->write(response);
+}
+
+std::string MetadataApiServer::SerializeMetricsToPrometheusTextFormat() const {


I would suggest creating a Telemetry class to wrap this whole Collectable business and the registry and text serialization. Then you could have a field telemetry_ of type const Telemetry&, and this method can be replaced with telemetry_.MetricsToString() (or even telemetry_.ToString()).

in the current implementation, it is implemented as function inside google::internal namespace, https://github.com/Stackdriver/metadata-agent/pull/210/files#diff-bcca2b17db5d9ee49a78774f858b95e4R25 . Two reasons:

It can be implemented as a function, instead of a class

implement the function inside google::internal namespace enables us to test this function, but does not need to keep an member telemetry_ inside the MetadataApiServer class

src/agent.h

StevenYCChou · 2018-12-06T13:42:53Z

@g-easy can you take a brief look on this PR? I want to get your feedback on

integrating opencensus-cpp library with building system (CMake and make)
code structure on using opencensus-cpp
application testing strategy with opencensus-cpp

src/measures.cc

src/measures.h

src/oauth2.cc

src/measures.cc

src/api_server.h

test/internal/measures_utils_unittest.cc

As we add more metrics, some of the metrics are other objects - e.g. KubernetesUpdater. View registration should happen after all the metrics are accessed. The lifecycle of all the measure are registered outside of MetadataAgent, so move view registration to the last step in main function.

g-easy

Build system stuff looks reasonable.
Test looks great.
Couple of comments on initialization.

src/measures.h

src/measures.cc

g-easy · 2018-12-07T04:15:56Z

src/internal/measures_utils.cc

+
+namespace {
+
+::prometheus::TextSerializer text_serializer;


Are text_serializer and exporter trivially constructible? Maybe make them both static pointers inside SerializeMetricsToPrometheusTextFormat?

Or even create them every time SerializeMetricsToPrometheusTextFormat is called.

now creating objects every time when SerializeMetricsToPrometheusTextFormat is called, because I cannot make sure whether my implementation on static pointer is the right approach, so eventually switch to object approach.

As for now, SerializeMetricsToPrometheusTextFormat() will only be called when server is called on /metrics specifically, so I don't think it will have huge performance impact and don't want to optimize early without my solid knowledge on static pointer approach.

src/metadatad.cc

src/oauth2.cc

StevenYCChou · 2018-12-07T16:24:30Z

change from RFC to normal one, because this one has tests included. high level review is still welcome first so we can get feedback loop soon.

igorpeshansky

High-level comments. I did not look at the Makefile changes yet — those will take more effort to review.

.gitmodules

igorpeshansky · 2018-12-07T16:35:07Z

.gitmodules

+[submodule "lib/prometheus-cpp"]
+	path = lib/prometheus-cpp
+	url = https://github.com/jupp0r/prometheus-cpp
+[submodule "lib/opencensus-cpp"]


I'm always wary of targeting an untagged and unreleased commit. We can fix this later, but just wanted to note this as a concern.

acknowledged. I just follow the pattern we used on other submodules. Once opencensus-cpp offically release their new version, we should check in that specific version. for now, I targeted on a commit which includes all the functionality we need, including prometheus-exporter.

src/agent.cc

src/api_server.cc

igorpeshansky · 2018-12-07T16:57:11Z

src/internal/measures_utils.cc

+
+namespace {
+
+::prometheus::TextSerializer text_serializer;


Or even create them every time SerializeMetricsToPrometheusTextFormat is called.

src/oauth2.cc

test/oauth2_unittest.cc

src/metrics.cc

test/metrics_unittest.cc

…o not know they will match google style guide.

src/oauth2.cc

davidbtucker · 2018-12-11T22:04:30Z

src/metrics.h

+
+  // View Descriptor accessors. If the view descriptor variable is not
+  // initialized, these methods will initialize the variable.
+  static const ::opencensus::stats::ViewDescriptor


Can this be private?

no. I tried to implement GetGceApiRequestErrorsCumulativeViewIntData, but it the data cannot propogated to this view because 1. the view needs to be created before the metric is recorded, and 2. for testing, we need to call TestUtils::Flush() to propagate to view. If this functino is private, it cannot be used in oauth2_unittest. We may put it as private function and make a class in oauth2_unittest as friend class, but if we add more metrics across files, you will friend a couple more - so I am not sure that's what we want.

In order to create a view from the view descriptor, and run TestUtils::Flush(), I put this function as public function.

See my comment about ::opencensus::stats::StatsExporter::GetViewData().

Also note that the standard trick for accessing private functions in tests is to mark them protected in the implementation class, and add a subclass in the test that makes them public. This can be made to work on non-virtual and static functions as well.

test/oauth2_unittest.cc

igorpeshansky · 2018-12-13T15:50:17Z

src/metrics.h

+
+  // View Descriptor accessors. If the view descriptor variable is not
+  // initialized, these methods will initialize the variable.
+  static const ::opencensus::stats::ViewDescriptor


See my comment about ::opencensus::stats::StatsExporter::GetViewData().

test/metrics_unittest.cc

igorpeshansky · 2018-12-13T17:00:24Z

test/metrics_unittest.cc

+          .set_measure(measure_name)
+          .set_aggregation(opencensus::stats::Aggregation::Count());
+  // remove view and flush the result before the actual test.
+  ::opencensus::stats::StatsExporter::RemoveView(view_name);


If we clean up in line 57, why do we also need to clean up here?

To make the test self-contained, you need to avoid other views with view_name registered. In current unittest there is only one view called test_view, so it shouldn't be an issue; however if other view registered the view_name already, the data of the view will be aggregated on top of the original view.

The view on line 57 is to be a good citizen to clean up the view, so other test cases won't be affected by this test.

Hmm, can we make the view name unique to this test, then (e.g., by using __FILE__ and __LINE__)? It seems weird that the first thing we do is unregister a newly-created view.

test/metrics_unittest.cc

src/metrics.cc

igorpeshansky · 2018-12-13T17:37:52Z

test/oauth2_unittest.cc

+
+  auth.GetAuthHeaderValue();
+
+  // Flush records to propagate to views.


This comment belongs on the next statement, doesn't it?

Flush records to propagate to the view we first initialize in line 148 - this comment explains the next two lines.

igorpeshansky · 2018-12-13T17:43:11Z

test/metrics_unittest.cc

+                  "test_view_1 1.000000 [0-9]*\n"));
+
+  // clean up the view which we registered for export.
+  ::opencensus::stats::StatsExporter::RemoveView(view_name);


Can we reuse the Finally class from oauth2.cc?

yes, but won't do it because it's in another .cc file - in order to use it, we need to refactor, but I don't think it's good idea to add much complexity to this already complex PR.

I'd rather pay the cost of 10 lines of code duplication for a proper RAII pattern than risk the flakiness resulting from forgetting to clean this up in the future.

igorpeshansky · 2018-12-13T17:44:24Z

test/metrics_unittest.cc

+  ::opencensus::stats::StatsExporter::RemoveView(::google::Metrics::kGceApiRequestErrors);
+  ::opencensus::stats::testing::TestUtils::Flush();
+
+  ::google::Metrics::RecordGceApiRequestErrors(1, "test_kind");


How does this work? If you've removed the view from StatsExporter and never re-registered it, how are those values getting exported into SerializeMetricsToPrometheusTextFormat?

I don't understand - I think you suggested that when we register the measurement internally, we also register the view for it?

For test, to make this testcase not affected by other test, we need to remove the view first (first 2 lines) and let the logic inside RecordGceApiRequestErrorsregister the view and record the measurement, so later the value is exported by SerializeMetricsToPrometheusTextFormat

Yes, I did suggest that, but the fact that we remove the view and expect it to be re-registered threw me off a bit in this test. A comment to remind readers of this setup would be helpful. In fact, because the result of GceApiRequestErrorsInitialize is cached, would the view even be re-registered at all?

As for having to unregister the view in the first place, this strikes me as pretty fragile, and, as I said in another comment, gives me no confidence that we're testing what we're using in production. I can see a couple of ways of addressing this, the most sensible being to retrieve the value for this metric using ::opencensus::stats::StatsExporter::GetViewData()[1] before calling the method under test and do the same after calling the method under test, and check that the value changed in the expected way. We could then change this specific test to make sure that if we touch the GceApiRequestErrors counter, it appears in the prometheus output with some value. Does this make sense?

[1] I wish opencensus-cpp provided a way to retrieve the counter value by view name directly from the StatsExporter.

igorpeshansky · 2018-12-13T17:46:11Z

test/oauth2_unittest.cc

+
+  // Remove existing view on kGceApiRequestErrors to avoid
+  // other tests affecting the result.
+  // Flush to propagate existing records to views, including records from other


Does this comment belong on the next statement?

why it cannot be grouped into a section of comments?

test/metrics_unittest.cc

igorpeshansky · 2018-12-14T15:18:45Z

test/metrics_unittest.cc

+TEST(SerializeToPrometheusTextTest, SingleOpencensusView) {
+  const char measure_name[] = "test_measure";
+  const char view_name[] = "test_view";
+  const ::opencensus::stats::MeasureInt64 test_measure =


The proposed function could be reused verbatim to implement GceApiRequestErrors (instead of GceApiRequestErrorsInitialize)... Basically, I'm concerned about the amount of setup these tests are doing, and it seems like this is fragile in that it could easily diverge from the setup in production code. The closer we can get this to production code, the more confidence we would have that we're testing what we're using.

igorpeshansky · 2018-12-14T15:32:29Z

test/metrics_unittest.cc

+                  "test_view_1 1.000000 [0-9]*\n"));
+
+  // clean up the view which we registered for export.
+  ::opencensus::stats::StatsExporter::RemoveView(view_name);


I'd rather pay the cost of 10 lines of code duplication for a proper RAII pattern than risk the flakiness resulting from forgetting to clean this up in the future.

igorpeshansky · 2018-12-14T15:37:18Z

test/metrics_unittest.cc

+          .set_measure(measure_name)
+          .set_aggregation(opencensus::stats::Aggregation::Count());
+  // remove view and flush the result before the actual test.
+  ::opencensus::stats::StatsExporter::RemoveView(view_name);


Hmm, can we make the view name unique to this test, then (e.g., by using __FILE__ and __LINE__)? It seems weird that the first thing we do is unregister a newly-created view.

igorpeshansky · 2018-12-14T15:45:43Z

test/metrics_unittest.cc

+  ::opencensus::stats::StatsExporter::RemoveView(::google::Metrics::kGceApiRequestErrors);
+  ::opencensus::stats::testing::TestUtils::Flush();
+
+  ::google::Metrics::RecordGceApiRequestErrors(1, "test_kind");


Yes, I did suggest that, but the fact that we remove the view and expect it to be re-registered threw me off a bit in this test. A comment to remind readers of this setup would be helpful. In fact, because the result of GceApiRequestErrorsInitialize is cached, would the view even be re-registered at all?

As for having to unregister the view in the first place, this strikes me as pretty fragile, and, as I said in another comment, gives me no confidence that we're testing what we're using in production. I can see a couple of ways of addressing this, the most sensible being to retrieve the value for this metric using ::opencensus::stats::StatsExporter::GetViewData()[1] before calling the method under test and do the same after calling the method under test, and check that the value changed in the expected way. We could then change this specific test to make sure that if we touch the GceApiRequestErrors counter, it appears in the prometheus output with some value. Does this make sense?

[1] I wish opencensus-cpp provided a way to retrieve the counter value by view name directly from the StatsExporter.

igorpeshansky · 2018-12-14T15:48:14Z

test/oauth2_unittest.cc

+    std::string(test_info_->name()) + "_creds.json",
+    "{\"client_email\":\"user@example.com\",\"private_key\":\"some_key\"}");
+  Configuration config(std::istringstream(
+      "CredentialsFile: '" + credentials_file.FullPath().native() + "'\n"


Nit: should this be a 2-space indent?

igorpeshansky · 2018-12-14T15:52:07Z

test/oauth2_unittest.cc

+  // affecting the result, then flush to propagate existing records
+  // to views, including records from other tests.
+  ::opencensus::stats::StatsExporter::RemoveView(
+      ::google::Metrics::kGceApiRequestErrors);


We're inside the google namespace here. Just Metrics::kGceApiRequestErrors should do.

igorpeshansky · 2018-12-14T15:52:25Z

test/oauth2_unittest.cc

+  // StatsExporter::AddView always add a new view and destroy the old one,
+  // which is not suitable for our
+  // Metrics::GceApiRequestErrorsCumulativeViewDescriptor().
+  ::google::Metrics::GceApiRequestErrorsCumulativeViewDescriptor().


We're inside the google namespace here. Just Metrics::GceApiRequestErrorsCumulativeViewDescriptor() should do.

igorpeshansky · 2018-12-14T16:03:09Z

test/oauth2_unittest.cc

+              ::testing::Contains(::testing::Pair(
+                      Metrics::GceApiRequestErrorsCumulativeViewDescriptor(),
+                      ::testing::Property(
+                          &::opencensus::stats::ViewData::int_data,


Can we pull this out into Metrics::GetInt64Data(const std::string& name)? That way this test could become:

EXPECT_THAT(Metrics::GetInt64Data(Metrics::kGceApiRequestErrors), ::testing::UnorderedElementsAre( ::testing::Pair(::testing::ElementsAre("oauth2"), 1)));

igorpeshansky · 2018-12-14T16:04:35Z

src/metrics.h

+
+  // View Descriptor accessors. If the view descriptor variable is not
+  // initialized, these methods will initialize the variable.
+  static const ::opencensus::stats::ViewDescriptor


Also note that the standard trick for accessing private functions in tests is to mark them protected in the implementation class, and add a subclass in the test that makes them public. This can be made to work on non-virtual and static functions as well.

igorpeshansky · 2018-12-14T16:35:37Z

src/oauth2.h

 public:
  OAuth2(const Environment& environment)
-    : OAuth2(environment, ExpirationImpl<std::chrono::system_clock>::New()) {}
+      : OAuth2(environment, ExpirationImpl<std::chrono::system_clock>::New()) {}


Why did we change the indentation here? Should we just revert this file altogether?

Expose API server with metrics in prometheus format.

8ab1570

StevenYCChou requested review from davidbtucker and igorpeshansky November 15, 2018 19:40

davidbtucker approved these changes Nov 15, 2018

View reviewed changes

src/Makefile Outdated Show resolved Hide resolved

src/api_server.cc Outdated Show resolved Hide resolved

test/Makefile Outdated Show resolved Hide resolved

test/api_server_unittest.cc Outdated Show resolved Hide resolved

StevenYCChou added 3 commits November 16, 2018 11:34

Fix indentation and line wrapping.

2427bce

Remove a redundant test.

7b3f07e

Make indentation consistent in Makefile.

955a061

Fix typo.

12b4a86

igorpeshansky suggested changes Nov 21, 2018

View reviewed changes

StevenYCChou changed the title ~~Add instrumentation exposition into api server~~ RFC: Add instrumentation exposition into api server Nov 22, 2018

StevenYCChou added 2 commits December 6, 2018 05:52

Instrument first metric with opencensus-cpp.

5c065da

Makefile change

c667388

StevenYCChou changed the title ~~RFC: Add instrumentation exposition into api server~~ RFC: Add instrumentation and expose measurement in prometheus format Dec 6, 2018

davidbtucker reviewed Dec 6, 2018

View reviewed changes

StevenYCChou added 7 commits December 6, 2018 15:13

remove unused variable.

cf5f833

expand comments on functions in measures.h

b788df1

remove declaration on internal variable

e49c64d

make string declaration consistent

ec5a859

add comments on why accessing measure in OAuth2 constructor helps

bbbd851

Use the right header for prometheus compatible format.

ac7c9ea

g-easy reviewed Dec 7, 2018

View reviewed changes

StevenYCChou changed the title ~~RFC: Add instrumentation and expose measurement in prometheus format~~ Add instrumentation and expose measurement in prometheus format Dec 7, 2018

igorpeshansky suggested changes Dec 7, 2018

View reviewed changes

StevenYCChou added 3 commits December 7, 2018 12:47

Address comments on measurement organization.

5ac289e

fix format in test file.

d8b3e37

revert header/constructor because no need anymore.

93790ed

StevenYCChou added 8 commits December 7, 2018 14:14

Wrap opencensus related code into a class.

786245f

Move util function into Metrics Class.

c22faac

remove explicit dependency on prometheus-cpp.

cdfe9c1

Clean up prometheus-cpp dependency on gitmodules as well.

7514b25

Revert submodules on prometheus-cpp.

4f37882

Revert chnage for prometheus-cpp.

96b268a

Flush metrics only within one test, and add comments.

64929c8

Avoid using string_view for constant string.

88830d0

g-easy reviewed Dec 10, 2018

View reviewed changes

src/metrics.cc Show resolved Hide resolved

g-easy reviewed Dec 10, 2018

View reviewed changes

src/metrics.cc Outdated Show resolved Hide resolved

test/metrics_unittest.cc Show resolved Hide resolved

StevenYCChou and others added 7 commits December 10, 2018 07:11

Use constexpr consistently.

6ee8ab8

Refactor code, fix tests which is not idempotent.

bee8a32

initialize export related object every time to be safe - because we d…

2687728

…o not know they will match google style guide.

reorder variable definition.

5c6526e

Use constexpr const.

546958f

Add metrics tests on GCE Api Request Errors.

775737c

fix format.

fc935e6

davidbtucker approved these changes Dec 11, 2018

View reviewed changes

StevenYCChou added 2 commits December 11, 2018 20:47

Fix format on namespace.

1fe4649

Fix typo.

1099767

igorpeshansky suggested changes Dec 13, 2018

View reviewed changes

StevenYCChou added 2 commits December 14, 2018 03:44

Address feedbacks.

1e56d36

Add const.

d6c6ac3

igorpeshansky suggested changes Dec 14, 2018

View reviewed changes


		auth.GetAuthHeaderValue();

		// Flush records to propagate to views.

Add instrumentation and expose measurement in prometheus format #210

Are you sure you want to change the base?

Add instrumentation and expose measurement in prometheus format #210

Uh oh!

Conversation

StevenYCChou commented Nov 15, 2018

Uh oh!

davidbtucker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

StevenYCChou commented Nov 16, 2018

Uh oh!

igorpeshansky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

StevenYCChou commented Dec 6, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

g-easy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

StevenYCChou commented Dec 7, 2018

Uh oh!

igorpeshansky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!